-
Notifications
You must be signed in to change notification settings - Fork 6.3k
Open VSX switch, Part II #4319
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open VSX switch, Part II #4319
Conversation
✨ Coder.com for PR #4319 deployed! It will be updated on every commit.
- Host: https://codercom-gevr5kx02-codercom.vercel.app/docs/code-server
- Last deploy status: success
- Commit: 4ca05c2
- Workflow status: https://github.com/cdr/code-server/actions/runs/1444566168
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so exciting!!! 🎉
Great work @TeffenEllis 🚀
Requesting changes only to ask if you don't mind updating this reply from Ranger - it's a bot which auto-responds and closes issues labeled extension-request. I'm wondering if we should also remove the issue template and remove the bot response all together?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still support SERVICE_URL and ITEM_URL or is this a breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvm, saw the implementation, we may want to make a note in the changelog so we remember to update the enterprise product 📓 (or maybe we can use the old vars as fallbacks)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, the two env’s have been replaced with the single EXTENSIONS_GALLERY as Microsoft upstream is now defining several keys we’d need to override. IMO, the empty env ITEM_URL= code-server ... also had some tricky ergonomics.
658721a to
95394e4
Compare
I think we need to fix this failing unit test. I was going to use .toMatch but got lazy because that takes a regex I think and I forgot to add it in.
●くろまる /_static › disabled authentication › should return a 404 when a file is not provided expect(received).toContain(expected) // indexOf Expected substring: "not found" Received string: "Not Found" 45 | 46 | const content = await resp.json() > 47 | expect(content.error).toContain(NOT_FOUND.message) | ^ 48 | })
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good! Only putting "Request changes" to make sure we fix that unit test before approving and merging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah-HA! My PR for the e2e test fix needed this!
1e98c4e to
6a53c2b
Compare
Codecov Report
Merging #4319 (e7712cc) into main (1b60ef4) will not change coverage.
The diff coverage isn/a.
❗ Current head e7712cc differs from pull request most recent head 4ca05c2. Consider uploading reports for the commit 4ca05c2 to get more accurate results
Impacted file tree graph
@@ Coverage Diff @@ ## main #4319 +/- ## ======================================= Coverage 66.46% 66.46% ======================================= Files 30 30 Lines 1625 1625 Branches 330 330 ======================================= Hits 1080 1080 Misses 463 463 Partials 82 82
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered by Codecov. Last update 1b60ef4...4ca05c2. Read the comment docs.
Applied test changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👍
(P.S. automerge may not work - at least it hasn't been working for me)
If we fix the conflicts, is this ready to be merged?
- Remove extension related github configs. - Update tests to reflect new upstream behavior.
f68b493 to
4ca05c2
Compare
Rebase merges are not allowed on this repository
Uh oh!
There was an error while loading. Please reload this page.
This PR continues the work set in place by Oxy in #2659, migrating our default configuration extension marketplace to VSX. We’ve received reports of extensions refusing to install, or exhibiting odd behaviors. I believe this is one part due to outdated extensions in our marketplace, and another regarding Microsoft’s upstream changes to determine what makes an extension "web compatible."
See coder/vscode@5f00e79 for additional implementation details.
Closes #1473